Retry transient Windows atomic write failures - #2603
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces retry logic in LocalStorage._write_file to handle transient PermissionError exceptions during os.replace operations, which typically occur on Windows, and adds a corresponding unit test. The review feedback suggests restricting this retry behavior to Windows platforms (os.name == 'nt') to avoid unnecessary delays on non-Windows systems where such errors are usually permanent, and recommends mocking os.name in the unit test to ensure it passes across all platforms.
| except PermissionError: | ||
| if attempt == 4: | ||
| raise | ||
| time.sleep(0.01 * (attempt + 1)) |
There was a problem hiding this comment.
On non-Windows platforms (like Linux/macOS), a PermissionError during os.replace is typically a permanent permission issue (e.g., write permission denied) rather than a transient sharing violation. Retrying in these cases introduces unnecessary delays (up to 100ms) before raising the exception.
We should restrict this retry behavior to Windows (os.name == 'nt').
Note: _write_json_file (line 553) also uses os.replace and could suffer from the same transient Windows failures. Consider extracting this retry logic into a shared helper function (e.g., _safe_replace) so both _write_file and _write_json_file can reuse it.
| except PermissionError: | |
| if attempt == 4: | |
| raise | |
| time.sleep(0.01 * (attempt + 1)) | |
| except PermissionError: | |
| if os.name != "nt" or attempt == 4: | |
| raise | |
| time.sleep(0.01 * (attempt + 1)) |
| raise PermissionError("destination is temporarily locked") | ||
| return real_replace(source, destination) | ||
|
|
||
| monkeypatch.setattr(os, "replace", replace_with_transient_lock) |
There was a problem hiding this comment.
To ensure this test passes on non-Windows platforms (where os.name is not 'nt'), we should mock os.name to 'nt' using monkeypatch so that the retry logic is triggered and tested correctly.
| monkeypatch.setattr(os, "replace", replace_with_transient_lock) | |
| monkeypatch.setattr(os, "name", "nt") | |
| monkeypatch.setattr(os, "replace", replace_with_transient_lock) |
|
Can you sign the PR |
There was a problem hiding this comment.
Pull request overview
Adds retries for transient Windows permission failures during atomic local-storage writes.
Changes:
- Retries
os.replaceup to five times with incremental delays. - Adds regression coverage for a transient replacement failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
python/aibrix/aibrix/storage/local.py |
Adds retry logic for payload replacement. |
python/aibrix/tests/storage/test_local_storage.py |
Tests successful replacement after one retry. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for attempt in range(5): | ||
| try: | ||
| os.replace(tmp_path, path) | ||
| break | ||
| except PermissionError: | ||
| if attempt == 4: | ||
| raise | ||
| time.sleep(0.01 * (attempt + 1)) |
Closes #2573
Summary:
Tests: